Skip to content

feat(tui): flash banner for blocked actions + Shift+R clone & rerun#11

Open
superbusinesstools wants to merge 2 commits intooxgeneral:mainfrom
superbusinesstools:feat/tui-clone-and-run
Open

feat(tui): flash banner for blocked actions + Shift+R clone & rerun#11
superbusinesstools wants to merge 2 commits intooxgeneral:mainfrom
superbusinesstools:feat/tui-clone-and-run

Conversation

@superbusinesstools
Copy link
Copy Markdown

Summary

Fixes two related TUI papercuts around the R (run) action:

  1. Blocked R produced no visible feedback. Pressing R on a done/failed/cancelled task did add a "Cannot run — status is done" message, but it went into the activity feed (bottom-right, high-frequency scroll, filter-dependent) where it was effectively invisible. Users would press R repeatedly and see nothing happen.
  2. No way to rerun a finished task. The state machine treats done as terminal — no done → todo transition exists, and orch task retry only handles failed/cancelled. There was no TUI path to run a completed task again.

Changes

  • Flash banner (src/tui/App.tsx): transient 4s, bold, colored banner rendered directly above the command bar. Triggered on blocked r, and on clone start/success/failure. Impossible to miss — sits where your eyes already are after pressing a key.
  • Shift+R → clone & run: new TaskService.clone() method copies title, description, priority, assignee, labels, goalId, workspace_mode, review_criteria, scope, and max_attempts into a fresh todo task (attempts reset to 0). A new onCloneTask callback wires it through the TUI and dispatches the clone via the orchestrator. Works on any task status, including terminal ones — history of the original task is preserved.
  • Lowercase r keeps its existing run-only behavior; when blocked it now flashes the reason and hints press Shift+R to clone & rerun.

Why clone instead of allowing done → todo?

Mutating a done task in place would destroy its history (stats, timestamps, prior attempts) and contradict the state machine's
terminal-status invariant. Cloning is auditable, reversible, and matches how most task systems handle "run it again."

Test plan

  • npm run typecheck — clean
  • npm test -- test/unit/application/task-service.test.ts — 28 passed (2 new clone tests)
  • Full suite: 12 failed / 1913 passed — identical to baseline on main, no regressions
  • Manual: in orch tui, press r on a done task → yellow flash with Shift+R hint
  • Manual: press Shift+R on a done task → amber "Cloning…" then green "Cloned & dispatched"; new todo appears and gets picked up
  • Manual: press r on a todo task → existing run behavior still works

Pressing R on a done/failed/cancelled task previously did nothing
visible — the block message was buried in the activity feed. Adds a
transient flash banner above the command bar so blocked actions produce
unmissable feedback, and introduces Shift+R to clone a terminal task
into a fresh todo and dispatch it.

Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
@oxgeneral
Copy link
Copy Markdown
Owner

Really nice PR — the reasoning for cloning vs a done → todo transition is spot-on, and rebasing on the fresh main (including the new CODEOWNERS/SECURITY) is appreciated. Clean service method, reasonable tests, well-chosen field set.

Two should-fixes and a couple of nits. Nothing blocking.

🟡 Should-fix 1 — shallow-copy aliasing in clone()

src/domain/task.ts declares review_criteria: ReviewCriterion[] and scope: string[] — both arrays. In TaskService.clone() only labels is defensively copied:

labels: [...src.labels],            // ✅ copied
review_criteria: src.review_criteria, // 🐛 shared array reference
scope: src.scope,                     // 🐛 shared array reference

The clone and the source share the same array instances, so a later .push() on one will mutate the other. Suggested fix:

review_criteria: src.review_criteria ? [...src.review_criteria] : undefined,
scope: src.scope ? [...src.scope] : undefined,

If ReviewCriterion has nested mutable fields, a deep copy (or structuredClone) would be safer still — your call.

🟡 Should-fix 2 — misleading "Failed to clone" when dispatch fails after a successful clone

In src/cli/commands/tui.ts:

const onCloneTask = async (taskId: string) => {
  const cloned = await container.taskService.clone(taskId);   // ✅ succeeds
  await container.orchestrator.runTask(cloned.id);            // ❌ may fail (no agent, lock conflict, …)
  return cloned;
};

If runTask rejects — the task is cloned and sitting in todo, but the user sees a red flash "Failed to clone: …" and no indication the clone exists. Two options:

  • (a) Split the try/catch so success/failure messages are accurate — "Cloned; failed to dispatch: …"
  • (b) Don't auto-dispatch — let the user press r explicitly. Less magic, more honest semantics.

🔵 Nits

  • setTimeout(() => setFlash(null), 4000) — factor the 4s into a named constant (FLASH_DURATION_MS).
  • Right now a blocked r fires both addMessage(...) and showFlash(...) with the same text. That's loud — probably flash-only (transient, eye-catching) or addMessage-only is enough, not both.
  • No CLI equivalent for clone (orch task clone <id> / --rerun). TUI users get the feature, headless/CI users don't. Easy follow-up PR.
  • No component test for the Shift+R handler itself (hard with Ink, understandable). Your service-level tests cover the important invariants (terminal statuses, attempts: 0, field copying) — good coverage for the tricky bit.

❓ The "12 failed" on main

Full suite: 12 failed / 1913 passed — identical to baseline on main

CI on main is green, so locally-only failures worry me. Could you share the 12 failing test names? If it's just lock-concurrency and friends (known flaky, documented in CONTRIBUTING.md), we'll ignore. If it's something new, that's a useful bug report on its own.


Love the pace — three focused PRs in a day is a rare signal. Once the should-fixes land I'm happy to merge. The clone design in particular is exactly the right shape for this state machine 🙌

@oxgeneral
Copy link
Copy Markdown
Owner

One more follow-up from a second pass — mostly architectural, not blockers:

Flash banner duplicates ToastBanner

src/tui/components/ToastBanner.tsx already exists and provides exactly this: queued auto-dismissing notifications, per-type color/icon, fade animation, stacking cap. Adding a parallel flash state in App.tsx means two notification systems to maintain.

Simpler path: extend ToastType from 'done' | 'failed' | 'review' → add 'info' (or 'clone'), add a color/icon/duration entry, and on clone push a toast via the existing onDismiss/queue plumbing. That also gives you fade animation and stacking for free.

clone / retry field drift

task-service.ts:144 copies: title, description, priority, assignee, labels, max_attempts, workspace_mode, review_criteria, scope, goalId.

But Task also has depends_on and attachments — neither is copied, so clones silently lose dependencies and attachments. When a new Task field lands, both clone and retry need updates and it's easy to forget one.

Consider a small helper like buildCloneInput(src) or COPYABLE_TASK_FIELDS constant, so clone/retry share the field list.

Layering: onCloneTask = clone() + runTask()

In tui.ts the adapter composes two service calls. If clone succeeds but runTask fails, the user sees a misleading error about the clone action. A TaskService.cloneAndRun(id) (or an orchestrator-level method) would keep the orchestration in the application layer and make the error boundary match the user's mental model.

Shallow copy on arrays (from earlier comment)

Still flagging review_criteria and scope — both are arrays passed by reference in clone. Labels already gets [...src.labels]. Same treatment needed.

Again — nothing here is a merge blocker on top of the earlier points, just worth considering.

…rror clarity, flash→toast

- task-service: spread review_criteria, scope, and depends_on arrays in clone() to
  prevent shared-reference mutation between clone and source
- tui.ts: wrap runTask in try/catch and attach `cloned` to dispatch errors so App
  can distinguish "clone failed" from "cloned but dispatch failed"
- App.tsx: replace parallel flash state with showInfoToast() routed through the
  existing ToastBanner queue; fixes duplicate addMessage+flash on blocked R;
  Shift+R error handler now shows accurate "dispatch failed" vs "clone failed" messages
- ToastBanner: add 'info' type (amber, 4s) with optional message override field
  so info toasts can carry arbitrary text alongside the existing done/failed/review types

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants